Feature parquet extract module - #126

DIReview Summary

3 critical | 5 warning(s) | 0 suggestion(s)

⚠️ Spec check skipped — tracker lookup failed (NO_REF_FOUND). Standards axis only.

DIReview Summary

0 critical | 2 warning(s) | 0 suggestion(s)

⚠️ Spec check skipped — tracker lookup failed (NO_REF_FOUND). Standards axis only.

DIReview Summary

0 critical | 1 warning(s) | 0 suggestion(s)

⚠️ Spec check skipped — tracker lookup failed (NO_REF_FOUND). Standards axis only.

DIReview Summary

1 critical | 0 warning(s) | 0 suggestion(s)

⚠️ Spec check skipped — tracker lookup failed (NO_REF_FOUND). Standards axis only.

DIReview Summary

1 critical | 1 warning(s) | 0 suggestion(s)

⚠️ Spec check skipped — tracker lookup failed (NO_REF_FOUND). Standards axis only.

DIReview Summary

0 critical | 1 warning(s) | 0 suggestion(s)

⚠️ Spec check skipped — tracker lookup failed (NO_REF_FOUND). Standards axis only.

DIReview Summary

0 critical | 1 warning(s) | 1 suggestion(s)

⚠️ Spec check skipped — tracker lookup failed (NO_REF_FOUND). Standards axis only.

Suggestions

• di/pqx/pqx.q:284 — The variable onDiskKeys is built with distinct over per-file extracted key lists, but when files agree on keys the result is a 1-element list whose sole element is itself a list of symbols. first onDiskKeys is then that symbol list, and the comparison not levels~first onDiskKeys works. However, when splits is a single-element list, {[idx;x] \$first each "=" vs' x idx}[idx] each splitsreturns a list containing one symbol list, sodistincton a 1-item list returns that same 1-item list — this part is fine. The real problem is the earlierdepths:count each splitsline:splitsis passed asexec split from files, which is a list of lists (each element is the split path segments for one file). count each splitsgives the number of segments per file — correct. But(first[depths]-1)-lvcomputes the number of hive levels as(segments_in_first_file - 1) - lv. The -1is intended to exclude the filename itself, butlvis already1 + count where "/"=string path, where pathis the base hive root (e.g. ``:pqxout14/ ``). The split segments for a file path like pqxout14/pqxtrade/date=2025.08.01/exch=N/f.parquet will have 5 elements; `lv` for a single-level base path is 1, so `depth = (5-1)-1 = 3`, but only 2 levels (`date`, `exch`) are present — off by one. Fix: use `(first[depths]-1)-lv` only if `lv` counts exactly the non-level prefix segments, or rewrite as `(first[depths]-1) - lv` and verify `lv` is computed consistently with the number of prefix segments in `splits`.

DIReview Summary

0 critical | 1 warning(s) | 0 suggestion(s)

⚠️ Spec check skipped — tracker lookup failed (NO_REF_FOUND). Standards axis only.

DIReview Summary

0 critical | 0 warning(s) | 2 suggestion(s)

⚠️ Spec check skipped — tracker lookup failed (NO_REF_FOUND). Standards axis only.

Suggestions

• di/pqx/pqx.q:281 — The checkvirtualtypes function never returns a value on the success path. In q, a function with no explicit return falls off the end returning the last expression's value — here that is the result of if[count bad;...], which on the success path is a generic null (::). This is benign if callers ignore the return, but it is inconsistent with the repo style guide's instruction to use explicit return (:) and may surprise a caller who checks the result. Add : (or : ::) at the end of the success path to make the intent explicit.
• di/pqx/pqx.q:271 — levelcols is built with .' flip (types;lv+til count names), which uses the Apply-Each (.) iterator over pairs of (typ; index). But castvirtuallevel takes THREE arguments [typ;x;y] — the third argument y (the files table row) is not passed here; it is meant to be a projection (partial application) that will be applied later in the functional-update ![files;...]. The projection castvirtuallevel[typ;x;] leaving y open is what castvirtualcol previously used. The new .' flip (types;lv+til count names) will call castvirtuallevel[typ;x] with only two args, producing a projection, not an error — so levelcols becomes a list of projections, which is structurally the same as before. However, verify that .' flip (types;lv+til count names) actually produces castvirtuallevel[types[i];(lv+i);] projections for each i, because . (Apply) on a list of 2 elements calls castvirtuallevel with those 2 elements as positional args (i.e. castvirtuallevel[typ;x]), which IS a valid 2-arg partial application leaving y open. This appears correct — but the old code used {[typ;x] (castvirtuallevel[typ;x;];split)}as a lambda explicitly forming the projection and pairing it with ``split . The new code does `.' flip (types;lv+til count names)` which only produces the `castvirtuallevel[typ;x]` projections WITHOUT pairing them with split ``. The `` split column reference needed for the functional-update is missing from `levelcols`. In the functional update `![files;();0b;(`file,names)!enlist[...],levelcols]`, each element of `levelcols` should be a PARSE-TREE expression like `(castvirtuallevel[typ;x;];`split)`, not a bare projection. Bare projections are not valid parse-tree column expressions for `!` functional update — they must be wrapped as `(f;col)`. The old lambda `{[datecol;x] (castvirtualcol[datecol;x;];`split)}[datecol] each lv+til count levels` explicitly built `(projection;`split)` pairs; the new code drops the `split `` wrapping, breaking the functional update.

DIReview Summary

0 critical | 1 warning(s) | 1 suggestion(s)

⚠️ Spec check skipped — tracker lookup failed (NO_REF_FOUND). Standards axis only.

Suggestions

• di/pqx/pqx.q:305 — The depth and subsequent if checks are now reachable even when the earlier if[1<count distinct depths;...] branch throws — that is correct — but depth:(first[depths]-1)-lv; is computed AFTER the early-exit if block closes. However, when count splits is 0 (no files found), depths is an empty list, first[depths] returns a null (0N), and the subtraction proceeds silently producing a null rather than an error, then if[depth<>count levels;...] compares null to an int. This null-check guard was not present before and is not added here. Add a guard: if[0=count splits; .z.m.logerr[...]; 'err] before accessing first depths.